Skip to content

fix: resolve issue with add overwriting existing files #92#101

Merged
S4tvara merged 4 commits intoS4tvara:mainfrom
Udayan853:dev
Oct 6, 2025
Merged

fix: resolve issue with add overwriting existing files #92#101
S4tvara merged 4 commits intoS4tvara:mainfrom
Udayan853:dev

Conversation

@Udayan853
Copy link
Copy Markdown
Contributor

@Udayan853 Udayan853 commented Oct 5, 2025 •
edited
Loading

Changes

  • Updated builder.go to ask for confirmation when overwriting existing vault files (skip by default)
  • Updated add.go to ignore skipped files
  • Updated delete.go to be consistent with new .yaml file convention
  • Updated vault files to follow <location>.<filename>.yaml convention to avoid conflicts with duplicate files

Fixes #92

@Udayan853
fix: resolve issue with add overwriting existing files
006e9f7 
- Updated `builder.go` to ask for confirmation when overwriting existing
  vault files (skip overwriting by default)
- Updated `add.go` to ignore skipped files
- Updated vault files to follow `<location>.<filename>.yaml`
  convention to avoid conflicts with duplicate files
@Udayan853
Merge branch 'main' into dev
196d811
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 5, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 32.14286% with 19 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/manifest/builder.go 0.00% 12 Missing ⚠️
cmd/add.go 0.00% 4 Missing ⚠️
cmd/delete.go 0.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@S4tvara
Copy link
Copy Markdown
Owner

S4tvara commented Oct 5, 2025

Hi @Udayan853 ,

please add proper test coverage,
rest looks fine to me

Cheers,
Nilay

@Udayan853
refactor: Move confirmation dialog to util
5c87a85 
- Added unit test for `ConfirmOverwrite`
- Modified `StoreFileManifest` in `builder.go` to use `ConfirmOverwrite`
@Udayan853
Merge branch 'main' into dev
a1258ff
@Udayan853
Copy link
Copy Markdown
Contributor Author

Udayan853 commented Oct 6, 2025

Hey @SubstantialCattle5

I have added a unit test for confirmation prompt,
However, testing the newly added code block for proper coverage in builder.go, delete.go and add.go would require a significant refactor since the surrounding functions have a lot of side effects
Wouldn't this be considered a separate issue? Or should I proceed to make changes in this PR itself?

@Udayan853
Copy link
Copy Markdown
Contributor Author

Udayan853 commented Oct 6, 2025

Ah, nevermind -- I initially thought that 100% coverage would be needed, since only 11.60% is needed I suppose this should be good

@S4tvara
Copy link
Copy Markdown
Owner

S4tvara commented Oct 6, 2025

@Udayan853 ,
thanks for the contribution. In office right now, I'll merge it later after review it

Cheers!
Nilay

@S4tvara
Copy link
Copy Markdown
Owner

S4tvara commented Oct 6, 2025

Hey @SubstantialCattle5

I have added a unit test for confirmation prompt, However, testing the newly added code block for proper coverage in builder.go, delete.go and add.go would require a significant refactor since the surrounding functions have a lot of side effects Wouldn't this be considered a separate issue? Or should I proceed to make changes in this PR itself?

I do need help with total test coverage lemme know if you wanna pick that up!

@S4tvara S4tvara changed the title fix: resolve issue with add overwriting existing files fix: resolve issue with add overwriting existing files #92 Oct 6, 2025
@Udayan853
Copy link
Copy Markdown
Contributor Author

Udayan853 commented Oct 6, 2025

I do need help with total test coverage lemme know if you wanna pick that up!

This has already been taken up in #69.
I'd be happy to help with other unassigned issues though :)

@S4tvara S4tvara merged commit bc6bc8c into S4tvara:main Oct 6, 2025
38 checks passed
@S4tvara
Copy link
Copy Markdown
Owner

S4tvara commented Oct 6, 2025

@Udayan853,
I’ve reviewed the code and found no issues.
Merging your PR.

Cheers,
Nilay

@S4tvara S4tvara mentioned this pull request Oct 6, 2025
Closed
@S4tvara
Copy link
Copy Markdown
Owner

S4tvara commented Oct 6, 2025 •
edited
Loading

I do need help with total test coverage lemme know if you wanna pick that up!

This has already been taken up in #69. I'd be happy to help with other unassigned issues though :)

@Udayan853,
The issue has been raised but no one has been assigned yet. Send a req over there if you wanna pick it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

hacktoberfest hacktoberfest-accepted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The list file operation sietch ls doesn't take into account the dedup chunk index.

2 participants

@Udayan853 @S4tvara